-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(directive): throw if output the same event more than once #5113
Conversation
if (isPresent(dm.outputs)) { | ||
dm.outputs.forEach((propName: string) => { | ||
if (ListWrapper.contains(outputs, propName)) { | ||
throw new BaseException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you include the type name as well in the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced selector with type name
152396e
to
cb5cbde
Compare
throw new BaseException( | ||
`Output the same event more than once: '${propName}' in '${stringify(this.type)}'`); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be that dm.OUTPUTS
contains the same event twice ? Should we check that too ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it. I decided not to check since outputs: ['eventA','eventA']
should looks weird.
07f0b46
to
4813c94
Compare
dm.outputs.forEach((propName: string) => { | ||
if (ListWrapper.contains(outputs, propName)) { | ||
throw new BaseException( | ||
`Output the same event more than once: '${propName}' in '${stringify(type)}'`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change the message to "Output event '${propName}' defined multiple times in '${stringify(type)}'
the rest looks good |
4813c94
to
3dece56
Compare
@IgorMinar updated |
3dece56
to
b56581e
Compare
Merge instructions: if problems are identified during merge & sync lets punt this to beta.1 // @alexeagle |
Merging PR #5113 on behalf of @alexeagle to branch presubmit-alexeagle-pr-5113. |
Merging PR #5113 on behalf of @IgorMinar to branch presubmit-IgorMinar-pr-5113. |
Merging PR #5113 on behalf of @rkirov to branch presubmit-rkirov-pr-5113. |
merged at 8c37b7e |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Close: #4798